fix: support bracketed IPv6 runtime URLs - #2200
Conversation
|
The underlying gap is worth fixing, but I think this PR should be narrower so it adds IPv6 without changing the existing endpoint grammar or taking on general URL-validation policy. A minimal version would be:
The intended new cases are simply: RuntimeConnection.forUri("[::1]:4321");
RuntimeConnection.forUri("http://[2001:db8::1]:4321");Tests should cover those forms in each URL-based SDK, plus one Python connector-level test proving the connection path is not forced to I would leave path/query/fragment rejection, whitespace normalization, arbitrary-scheme handling, userinfo policy, and unrelated port-validation changes out of this PR. Moving wholesale to general-purpose URL parsers changes which previously accepted inputs succeed or fail and introduces cross-language differences that then require us to define and maintain a much larger URL contract. A narrow bracketed-IPv6 branch fixes the demonstrated user problem, preserves compatibility, and avoids pulling those edge cases into scope. |
|
@xianjianlf2 Does that sound reasonable? I'll mark this PR as draft but please remark as ready to review as appropriate. |
b42ff60 to
44894de
Compare
|
Updated in 44894de after maintainer feedback:
Local validation passed for the Go unit packages, Node typecheck/format/lint, Python parsing/connection tests, and AIRI display tests. Please re-review. |
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
The narrow approach is now appropriate and the Python transport fix is necessary, but the new bracketed syntax is not consistently validated yet. Please address the three inline comments and add a negative regression case for [not-ipv6]:1234 in Node, Python, and Go.
Also update the PR description before marking it ready again: the summary still says path/query/fragment forms are rejected even though that change was intentionally removed, and it omits Java from the affected SDKs. The branch is currently behind main and has no CI results, so update it onto current main and let the normal checks complete before requesting re-review.
| if (isNaN(port) || port <= 0 || port > 65535) { | ||
| throw new Error(`Invalid port in cliUrl: ${url}`); | ||
| } | ||
| return { host: ipv6Match[1], port }; |
There was a problem hiding this comment.
This regex accepts any non-] text, so [not-ipv6]:1234 becomes host not-ipv6. That conflicts with the documented [ipv6]:port grammar and with .NET/Java, which reject this input. Validate ipv6Match[1] with Node's net.isIPv6 before returning, and add a negative regression test. Scoped literals such as fe80::1%eth0 should remain supported.
| raise ValueError(f"Invalid cli_url format: {url}") | ||
| ipv6_match = re.match(r"^\[([^\]]+)\]:(.*)$", clean_url) | ||
| if ipv6_match: | ||
| host = ipv6_match.group(1) |
There was a problem hiding this comment.
The bracket payload is treated as IPv6 without validation, so [not-ipv6]:1234 is accepted. Validate it with Python's standard IPv6 parser (while preserving scoped literals) and add a negative regression test. This should match the .NET/Java behavior for the same public endpoint syntax.
| if err != nil || port <= 0 || port > 65535 { | ||
| panic(fmt.Sprintf("Invalid port in URIConnection: %s", url)) | ||
| } | ||
| return host, port |
There was a problem hiding this comment.
net.SplitHostPort only splits the bracketed authority; it does not prove host is IPv6, so [not-ipv6]:1234 is accepted. Validate the returned host with net/netip.ParseAddr and require Is6() before returning, preserving IPv6 zones, then add a negative regression test.
2 participants
Fixes #1137
Summary
Validation
cd go && go test .cd python && uv run pytest test_client.py -qcd nodejs && npx vitest run test/client.test.ts -t "URL parsing"cd dotnet && dotnet test test/GitHub.Copilot.SDK.Test.csproj --filter RuntimeConnectionUrlParsingTests(dotnetis not installed in this environment:zsh:1: command not found: dotnet)